ADFA-5242: Retry an accept() failure instead of shutting the server down - #1728

ServerSocket.accept() is declared to throw IOException, of which
SocketException is one subtype. The accept loop caught only that subtype, and
the enclosing try has a finally but no catch, so any other IOException unwound
past the loop to start()'s outermost handler -- whose finally closes the
listening socket *and* the database. Every later documentation request then
failed until the app restarted, with a single "Error: ..." line as the only
trace.

The realistic trigger is descriptor exhaustion, which is self-limiting in the
worst way: the descriptors accept() is waiting for are held by this server's own
in-flight connections, so the condition clears moments later -- by which point
the server has already shut itself down.

Now only the listening socket closing ends the loop, as its own named predicate:
getting this wrong fails differently in each direction, and treating a transient
failure as terminal is exactly the bug being fixed. Non-fatal failures log their
exception type and retry after 50 ms, so a persistent failure cannot spin the
loop at full tilt, flooding the log and competing with the connection closes
that would fix it. A successful accept never waits.

Found by CodeRabbit on PR #1688, whose ADFA-5172 instrumentation is abandoned;
the defect it pointed at is in stage regardless, which is why this is a separate
change against stage's own loop rather than a rescue of that branch.

Three tests on the predicate: both spellings of a closed socket, a reset
connection, a SocketTimeoutException, descriptor exhaustion, and -- since the
close is identified only by its message -- exceptions with no message at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…itten for

The review is right: shouldStopAccepting and pauseAfterFailedAccept were
reachable only from their unit tests. start() still caught SocketException and
still retried without a backoff, so the fix this branch claims to make did not
exist in the running server -- a bare IOException such as "Too many open files"
went on unwinding to start()'s outermost handler, whose finally closes the
listening socket and the database.

The loop now catches IOException, breaks only when shouldStopAccepting says the
socket closed, and pauses before retrying anything else.

The accept loop moves out of start() into an internal acceptLoop(ServerSocket).
That is what makes the behaviour testable: the rest of start() needs a live
Android runtime -- TrafficStats, SQLite -- while the loop needs neither, which is
why nothing exercised it before. Two tests now drive it through a ServerSocket
whose accept() fails on demand; the retry test fails against the previous loop
with the IOException escaping, which is the defect itself.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…essage text

Three defects from reviewing my own PR.

A 50 ms backoff bounds CPU, not volume: a permanent failure retried forever at
20 log lines a second, and the comment claimed the backoff prevented flooding
the log. On this phone's 5 MiB logcat buffer that one line displaces every other
diagnostic within the hour. After 20 consecutive failures the loop now gives up,
having said so once; any successful accept resets the count, so unrelated
failures over a long session cannot accumulate into a shutdown.

shouldStopAccepting matches the exception's message. stopRequested is
authoritative and message-independent, and is now checked first: if a platform
ever words a closed socket differently, matching text alone would spin until the
cap instead of exiting, leaving start()'s finally unrun -- the database open and
the port held, which is worse than the failure this method exists to survive.

stopRequested is @volatile now that the accept loop reads it without the lock.

Three tests: the cap, the reset, and the stop flag. The last fails at 20 instead
of 1 without its fix; a negative test for the cap would hang the build, which is
the defect it prevents.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

A blank line before the @volatile comment, and the indentation of a KDoc the
pre-push hook's spotlessApply corrected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…tate

jatezzz: the loop was a try inside a try inside a while, with a finally around
both. It is now three functions doing one thing each -- acceptLoop, serveThenClose,
reportClientFailure -- with a single level of try in each and the accept result
read as an expression.

CodeRabbit: shouldStopAccepting matched the exception's message. It now reads
stopRequested and socket.isClosed. ServerSocket.close() sets that flag before
accept() unblocks, so the state is both authoritative and available, where the
message was a guess about wording. A closed-sounding message from a socket that
is still open is now retried like any other fault, which has a test.

CodeRabbit: the throwable is passed to SLF4J rather than interpolated, so an
unexpected accept failure carries its stack trace. The --DS note about
placeholders concerned interpolation into the message; a trailing throwable
argument is the idiom SLF4J is asking for, and I was wrong to decline this
earlier for consistency with the interpolated lines.

Eight tests, all driving the real loop: the cap, the reset, a closed socket, a
stop before the socket closes, a closed-sounding message retried, and every
IOException subtype retried.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…he loop

Review of #1728 found the retry could still end in the outcome this
ticket exists to prevent, by three routes.

The 20-failure cap was the main one. Hitting it broke out of the accept
loop, which returns to start(), whose finally closes the listening
socket AND the database -- documentation dead for the rest of the
process. That is the ADFA-5242 symptom, delayed by a second. The cap
existed to bound log volume, so the interval now does that job instead:
50 ms doubling to a 2 s ceiling, reached in eight failures, which is
well under a line a second against the twenty a fixed 50 ms retry
produced. The stack trace goes out once per burst rather than nineteen
times, and repeats only when the interval escalates. Nothing ends the
loop now except the socket closing or an interrupt.

Second, client.close() sat unguarded in serveThenClose's finally, and
the call was outside the loop's try. Socket.close() is declared to
throw, and a client that resets mid-response makes it do so -- from a
finally, so it replaced any in-flight exception and unwound into
start(). One bad client took documentation down by the same route as
the accept failure. It is caught now, and the loop guards the call as
well.

Third, an interrupt during the backoff re-armed the flag and continued,
so every later sleep threw immediately and the loop hot-spun through
its retries at full CPU -- the opposite of the backoff's purpose. An
interrupt now ends the loop.

Three comments claimed things that were not true and are corrected:
stop() then start() was never a recovery path (stopRequested is
one-way by design, so a stop before bind cannot leave an orphaned
listener; MainActivity constructs a fresh WebServer per start, which is
the actual restart), libcore's ServerSocket.close() sets its closed
flag AFTER impl.close() so isClosed can still be false when accept()
unblocks -- stopRequested is what carries the decision -- and the
descriptor pressure is not self-inflicted, since handleClient runs
inline and the server holds two descriptors at most.

The backoff sleep is now injectable. The four tests that drove the cap
spent 4.25 s in real Thread.sleep and asserted a bare 20; the suite now
records the intervals instead, so it asserts the escalation and the
reset by name and runs in 0.2 s. Six of the eleven cases fail against
the unfixed code, each for the reason it is named for.

Found in review of PR #1728.

…e on repeats

Two CodeRabbit findings on the accept loop.

retriesAtCeiling was cleared only when a success brought backoffMs all
the way to zero. The counter means "consecutive retries at the ceiling",
and a successful accept ends that run whatever interval is left, so the
old rule banked a stale count: at the ceiling with 14 retries recorded,
one success followed by a return to the ceiling fired the 30-second
heartbeat on the very next retry instead of the fifteenth.

The repeat log lines carried e.message only. That was deliberate -- the
stack trace goes out once per burst and repeats stay terse -- but the
type went with it. A burst can change cause mid-flight (EMFILE giving
way to ECONNABORTED) and the two lines would read identically, and
message is null for some IOExceptions, which logged a bare "null". They
now carry e.toString(), which keeps the type without the trace.

Not covered by a test: both are logging cadence, and the app module's
test classpath has slf4j-api with no provider, so nothing observes a log
line. Adding a backend is a new dependency. The 12 existing
AcceptFailureTest cases still pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU